Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use constructors for static tokens #29

Closed
wants to merge 1 commit into from
Closed

Use constructors for static tokens #29

wants to merge 1 commit into from

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Apr 12, 2024 •
edited
Loading

WARNING: This commit includes breaking changes.

Instead of global variables, use function constructors for static tokens such as Null, ObjectStart, ObjectEnd, ArrayStart, and ArrayEnd. This provides a greater degree of immutability for the API.

Note that static tokens for True and False are dropped in favor of the pre-existing Bool constructor.
If necessary, we can always re-introduce True and False.

@dsnet dsnet requested review from mvdan and johanbrandhorst April 12, 2024 04:58
mvdan
mvdan approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I might have left it as it was before - lots of std packages have the immuntability issue with e.g. var ErrFoo = ..., and in practice it's not an issue. But SGTM either way.

@dsnet
Copy link
Collaborator Author

dsnet commented Apr 12, 2024

One advantage of this is that it makes Null at the same level of visibility as Bool, Int, etc.

In godoc, variables are given less prominence than functions (e.g., variables never show up in the table of contents, while functions do).

johanbrandhorst
johanbrandhorst approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like this better even if historically I take the point Dan makes

@ChrisHines
Copy link

I think it is fine. We did the same thing for "constant" values in go-kit/log/level.

https://github.com/go-kit/log/blob/0b69c7049332e99c25d5fd0f4d08317cfe45e7d8/level/level.go#L204-L239

@mvdan
Copy link
Collaborator

mvdan commented Apr 18, 2024

SGTM with the assumption the compiler is/will be clever enough to handle this efficiently

@Zxilly
Copy link

Zxilly commented Jul 3, 2024

I prefer the old scheme. If it's for immutability, can we set these enums to const string and expose the rawToken helper function?

@dsnet
Copy link
Collaborator Author

dsnet commented Jan 22, 2025 •
edited
Loading

The performance difference due to this change is largely inconsequential.

It seems to me that the benefits are:

  • Tokens constructors are now immutables
  • GoDoc more readily surfaces the existence of Null, StartObject, etc.
  • Arguably this is more consistent with the other constructors (e.g., String, Int, etc.)

The detriments are:

  • You now need to call constructor functions, which is two extra characters.

All the benefits and detriments are fairly minor and I'm still on the fence whether this is the right API change. Am I missing anything else?

@dsnet
Use constructors for static tokens
8b9bf14 
WARNING: This commit includes breaking changes.

Instead of global variables, use function constructors
for static tokens such as Null, ObjectStart, ObjectEnd,
ArrayStart, and ArrayEnd. This provides a greater degree
of immutability for the API.

Note that static tokens for True and False are dropped
in favor of the pre-existing Bool constructor.
If necessary, we can already re-introduce True and False.

Performance:

	Testdata/CanadaGeometry/Marshal/Concrete        888µs ± 2%     888µs ± 1%    ~     (p=0.670 n=43+45)
	Testdata/CanadaGeometry/Marshal/Interface       940µs ± 1%     939µs ± 1%    ~     (p=0.365 n=46+45)
	Testdata/CanadaGeometry/Unmarshal/Concrete     1.20ms ± 2%    1.20ms ± 2%    ~     (p=0.779 n=45+45)
	Testdata/CanadaGeometry/Unmarshal/Interface    1.67ms ± 1%    1.69ms ± 2%  +1.02%  (p=0.000 n=41+45)
	Testdata/CitmCatalog/Marshal/Concrete           881µs ± 1%     887µs ± 1%  +0.69%  (p=0.000 n=47+43)
	Testdata/CitmCatalog/Marshal/Interface         1.45ms ± 1%    1.45ms ± 2%  +0.39%  (p=0.031 n=45+46)
	Testdata/CitmCatalog/Unmarshal/Concrete        2.02ms ± 2%    2.01ms ± 2%    ~     (p=0.959 n=45+46)
	Testdata/CitmCatalog/Unmarshal/Interface       3.52ms ± 5%    3.58ms ± 2%  +1.71%  (p=0.000 n=46+45)
	Testdata/GolangSource/Marshal/Concrete         3.28ms ± 3%    3.32ms ± 4%  +0.99%  (p=0.004 n=46+47)
	Testdata/GolangSource/Marshal/Interface        4.67ms ± 3%    4.67ms ± 4%    ~     (p=0.116 n=46+46)
	Testdata/GolangSource/Unmarshal/Concrete       5.54ms ± 2%    5.51ms ± 1%  -0.60%  (p=0.021 n=44+41)
	Testdata/GolangSource/Unmarshal/Interface      8.51ms ± 3%    8.68ms ± 3%  +1.99%  (p=0.000 n=42+39)
	Testdata/StringUnicode/Marshal/Concrete        16.0µs ± 3%    15.9µs ± 4%  -0.56%  (p=0.030 n=48+47)
	Testdata/StringUnicode/Marshal/Interface       19.2µs ± 3%    18.6µs ± 5%  -3.52%  (p=0.000 n=47+47)
	Testdata/StringUnicode/Unmarshal/Concrete      20.7µs ± 4%    20.4µs ± 3%  -1.83%  (p=0.000 n=46+47)
	Testdata/StringUnicode/Unmarshal/Interface     26.2µs ± 4%    26.0µs ± 3%    ~     (p=0.101 n=48+50)
	Testdata/SyntheaFhir/Marshal/Concrete          5.60ms ± 3%    5.72ms ± 5%  +2.17%  (p=0.000 n=49+50)
	Testdata/SyntheaFhir/Marshal/Interface         2.32ms ± 4%    2.34ms ± 3%  +0.74%  (p=0.004 n=47+47)
	Testdata/SyntheaFhir/Unmarshal/Concrete        3.52ms ± 4%    3.53ms ± 3%    ~     (p=0.465 n=46+47)
	Testdata/SyntheaFhir/Unmarshal/Interface       5.72ms ± 7%    5.85ms ± 4%  +2.17%  (p=0.000 n=47+44)
	Testdata/TwitterStatus/Marshal/Concrete         605µs ± 5%     615µs ± 5%  +1.59%  (p=0.001 n=49+50)
	Testdata/TwitterStatus/Marshal/Interface        767µs ± 6%     759µs ± 4%  -1.04%  (p=0.005 n=50+50)
	Testdata/TwitterStatus/Unmarshal/Concrete      1.12ms ± 4%    1.12ms ± 2%    ~     (p=0.246 n=48+50)
	Testdata/TwitterStatus/Unmarshal/Interface     1.97ms ± 5%    1.99ms ± 5%  +0.84%  (p=0.026 n=47+49)
@mvdan
Copy link
Collaborator

mvdan commented Jan 23, 2025

I am also on the fence. Personally I'd lean towards not doing this for that reason; the solution adds a bit of complexity but doesn't bring substantial net benefit.

I continue to think that global variables in Go are OK; if they were a problem in practice, the language would be in trouble.

@dsnet
Copy link
Collaborator Author

dsnet commented Jan 23, 2025

I've decided to not pursue this change. During the formal proposal of "encoding/json/v2", if this issue is raised this again and sufficient number of people support it, we can pursue this again.

@dsnet dsnet closed this Jan 23, 2025
@dsnet dsnet deleted the immutable-token branch January 23, 2025 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdan mvdan mvdan approved these changes

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dsnet @ChrisHines @mvdan @Zxilly @johanbrandhorst